Skip to content

Adding consent manage PUT to update basic consent and authorisation resource with ID#911

Closed
Ashi1993 wants to merge 2 commits intowso2:mainfrom
Ashi1993:consent-put
Closed

Adding consent manage PUT to update basic consent and authorisation resource with ID#911
Ashi1993 wants to merge 2 commits intowso2:mainfrom
Ashi1993:consent-put

Conversation

@Ashi1993
Copy link
Contributor

@Ashi1993 Ashi1993 commented Mar 9, 2026

Adding consent manage PUT to update basic consent and authorisation resource with ID

This

Issue link: #909

Doc Issue: Optional, link issue from documentation repository

Applicable Labels: Spec, product, version, type (specify requested labels)


Development Checklist

  1. Build complete solution with pull request in place.
  2. Ran checkstyle plugin with pull request in place.
  3. Ran Findbugs plugin with pull request in place.
  4. Ran FindSecurityBugs plugin and verified report.
  5. Formatted code according to WSO2 code style.
  6. Have you verified the PR doesn't commit any keys, passwords, tokens, usernames, or other secrets?
  7. Migration scripts written (if applicable).
  8. Have you followed secure coding standards in WSO2 Secure Engineering Guidelines?

Testing Checklist

  1. Written unit tests.
  2. Verified tests in multiple database environments (if applicable).
  3. Tested with BI enabled (if applicable).

Summary by CodeRabbit

  • New Features

    • Enhanced consent update flows with pre- and post-update external processing, enriched responses, and authorization resource updates
    • New API contract exposing extension points for client, application, consent, token, authorize, and event workflows
  • Bug Fixes / Improvements

    • Improved internal request handling, standardized error messages, and more robust consent update persistence and mapping behavior
  • Tests

    • Expanded tests covering consent update success, validation, and error scenarios

@Ashi1993 Ashi1993 requested a review from Copilot March 9, 2026 15:14
@coderabbitai
Copy link

coderabbitai bot commented Mar 9, 2026

Walkthrough

Adds consent update flows with external pre-/post-processing: new service extension types, DTOs, utilities, core update API/implementation, handler wiring, SQL join change, tests and a large OpenAPI spec file.

Changes

Cohort / File(s) Summary
Service extension enum
financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/.../ServiceExtensionTypeEnum.java
Added PRE_PROCESS_CONSENT_UPDATE and ENRICH_CONSENT_UPDATE_RESPONSE enum constants.
External API DTOs (common)
.../consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.java, .../ExternalAPIBasicConsentResourceResponseDTO.java
Added basic request/response DTOs to represent minimal external consent resource payloads.
External API DTOs (manage)
.../manage/model/ExternalAPIPreConsentUpdateRequestDTO.java, .../PreConsentUpdateResponseDTO.java, .../PostConsentUpdateRequestDTO.java
Added pre/post update request/response DTOs for external update flows, including JSON serialization helpers.
Consent update DTOs & helpers
.../manage/model/ConsentManageAuthorizationUpdateDTO.java
Added DTO to represent authorization update results and mapping resources.
External API utilities
.../common/ExternalAPIUtil.java, .../manage/utils/ExternalAPIConsentManageUtils.java
Added builder to construct ConsentResource from basic external response and new methods to call external services for pre/post update extension points.
Consent extension utils
.../common/ConsentExtensionUtils.java
Added overloaded getInitiationResponse to build initiation responses from ConsentResource.
Consent manage utils
.../manage/utils/ConsentManageUtils.java, .../manage/utils/ConsentManageConstants.java
Added internal-request detection, full authorization update flow, helper methods for mapping management, and constants for internal header and client-id mismatch error.
Handler
.../manage/impl/DefaultConsentManageHandler.java
Wired external pre/post update flows into PUT path, added internal GET path handling, client ID validation and update persistence logic via new helpers and flags.
Core service API & impl
.../service/ConsentCoreService.java, .../service/impl/ConsentCoreServiceImpl.java, .../service/constants/ConsentCoreServiceConstants.java
Added updateConsent() to interface and implementation with transaction/audit handling; introduced update reason constant.
DAO query
.../dao/queries/ConsentMgtCommonDBQueries.java
Changed JOIN from RIGHT JOIN to LEFT JOIN when querying consent with attributes.
Tests & test utils
.../extensions/.../DefaultConsentManageHandlerTest.java, .../service/.../ConsentMgtCoreServiceTests.java, .../util/TestConstants.java, .../util/TestUtil.java, .../util/DataProviders.java
Extensive test updates: new/modified tests for update flows, new test constants and helpers, updated data providers to include expected error messages.
API spec
accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml
Added a large OpenAPI spec (new file) describing numerous extension endpoints and schemas.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler as ConsentHandler
    participant ExtSvc as ExternalService
    participant Core as ConsentCoreService
    participant DB as Database

    Client->>Handler: PUT /consent/{id} (payload + headers)
    Handler->>Handler: Validate clientId, path, internal-flag
    Handler->>Core: getConsent(consentId)
    Core->>DB: SELECT consent (+ attributes)
    DB-->>Core: consent resource
    Core-->>Handler: existing consent

    alt external pre-update enabled
        Handler->>ExtSvc: PRE_PROCESS_CONSENT_UPDATE (pre-update DTO)
        ExtSvc-->>Handler: pre-update response (possibly modified resource)
    end

    Handler->>Core: updateConsent(modified resource)
    Core->>DB: UPDATE consent, INSERT audit
    DB-->>Core: confirmation
    Core-->>Handler: updated consent

    alt external post-update enabled
        Handler->>ExtSvc: ENRICH_CONSENT_UPDATE_RESPONSE (post-update DTO)
        ExtSvc-->>Handler: enriched response
    end

    Handler-->>Client: 200 OK (updated/enriched response)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop through DTOs, enums, and flows,

Pre and post where new logic grows,
Mappings, calls, and tests aligned,
Consent updated — neat and kind. ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.34% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description is incomplete; the main content section starts with 'This' and trails off without explaining the purpose, and multiple required testing checklist items are unchecked or missing. Complete the purpose explanation beyond 'This', fill out all required sections, and clearly document test scenarios, automation test details, and conformance test results as specified in the template.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a PUT endpoint to update basic consent and authorization resources by ID, which aligns with the detailed changeset across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@wso2-engineering wso2-engineering bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

  • The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
  • Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.

✅ Before merging this pull request:

  • Review all AI-generated comments for accuracy and relevance.
  • Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Comment Accepted (Y/N) Reason
#### Log Improvement Suggestion No: 1
#### Log Improvement Suggestion No: 2
#### Log Improvement Suggestion No: 3
#### Log Improvement Suggestion No: 4
#### Log Improvement Suggestion No: 5
#### Log Improvement Suggestion No: 6
#### Log Improvement Suggestion No: 7
#### Log Improvement Suggestion No: 8
#### Log Improvement Suggestion No: 9
#### Log Improvement Suggestion No: 10
#### Log Improvement Suggestion No: 11
#### Log Improvement Suggestion No: 12
#### Log Improvement Suggestion No: 13
#### Log Improvement Suggestion No: 14
#### Log Improvement Suggestion No: 15
#### Log Improvement Suggestion No: 16
#### Log Improvement Suggestion No: 17
#### Log Improvement Suggestion No: 18
#### Log Improvement Suggestion No: 19
#### Log Improvement Suggestion No: 20

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements the consent management PUT endpoint in the WSO2 Financial Services Accelerator (issue #909), enabling toolkits to update existing basic consents and authorization resources through the accelerator.

Changes:

  • Added updateConsent method to ConsentCoreService/ConsentCoreServiceImpl to update basic consent data with audit tracking
  • Implemented handlePut in DefaultConsentManageHandler supporting both basic consent updates and internal authorization resource updates, with optional external service extension hooks
  • Added new DTOs, constants, and utility methods for pre/post consent update extension calls

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
ConsentCoreService.java New updateConsent interface method
ConsentCoreServiceImpl.java Implementation of updateConsent with DB transaction and audit record
ConsentCoreServiceConstants.java New BASIC_CONSENT_UPDATE_REASON constant
DefaultConsentManageHandler.java Full handlePut implementation; also bug fixes for CONSENT_RETRIEVECONSENT_CREATE/CONSENT_DELETE operation enums in handlePost/handleDelete; internal GET support
ConsentManageUtils.java New isInternalConsentRequest, updateConsentAuthResource, and related helper methods
ConsentManageConstants.java New INTERNAL_API_REQUEST_HEADER and CLIENT_ID_MISMATCH_ERROR constants
ExternalAPIConsentManageUtils.java New overloaded callExternalService for pre/post consent update
ExternalAPIUtil.java New buildConsentResource utility method
ConsentExtensionUtils.java New getInitiationResponse overload for ConsentResource
ExternalAPIBasicConsentResourceRequestDTO.java New DTO for basic consent request
ExternalAPIBasicConsentResourceResponseDTO.java New DTO for basic consent response
ExternalAPIPreConsentUpdateRequestDTO.java New pre-update external service request DTO
ExternalAPIPreConsentUpdateResponseDTO.java New pre-update external service response DTO
ExternalAPIPostConsentUpdateRequestDTO.java New post-update external service request DTO
ServiceExtensionTypeEnum.java Two new enum values for consent update extension types
DefaultConsentManageHandlerTest.java Tests for new handlePut functionality; test refactor from expectedExceptions to try-catch
ConsentMgtCoreServiceTests.java Tests for new updateConsent method
TestUtil.java / TestConstants.java / DataProviders.java Test utilities and constants for new and refactored tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Nitpick comments (2)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/DataProviders.java (1)

48-106: Remove or add tests for unused payload constants.

These constants are defined but never used elsewhere in the codebase:

  • INITIATION_WITHOUT_PERMISSIONS (line 48)
  • INITIATION_WITH_NON_STRING_PERMISSION (line 52)
  • INITIATION_WITHOUT_TRANS_FROM_DATES (line 89)
  • INITIATION_WITHOUT_TRANS_TO_DATES (line 103)

Either remove them if they're not needed, or add test cases if they represent important validation scenarios for permissions and date handling.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/DataProviders.java`
around lines 48 - 106, The four unused payload constants in DataProviders
(INITIATION_WITHOUT_PERMISSIONS, INITIATION_WITH_NON_STRING_PERMISSION,
INITIATION_WITHOUT_TRANS_FROM_DATES, INITIATION_WITHOUT_TRANS_TO_DATES) should
be either removed or exercised by tests; update the DataProviders class
accordingly by deleting the unused constants if they’re obsolete, or add unit
tests that reference these constants (e.g., in the consent validation test
suite) to cover missing-permissions, non-string-permission, missing
TransactionFromDateTime, and missing TransactionToDateTime scenarios so the
constants are actually used.
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java (1)

123-143: Extract the common initiation-response mutation into one helper.

This overload is byte-for-byte identical to the DetailedConsentResource variant, so future response changes can easily diverge between the two paths.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java`
around lines 123 - 143, The getInitiationResponse(Object responseObj,
ConsentResource createdConsent) implementation duplicates the same JSON mutation
performed by the DetailedConsentResource overload; extract the shared logic into
a private helper (e.g., populateInitiationData or buildInitiationData) that
accepts the JSONObject response (or its dataObject) and the ConsentResource,
moves the put(...) calls for CONSENT_ID, CREATION_DATE_TIME,
STATUS_UPDATE_DATE_TIME and STATUS, and the remove/put of DATA into that helper,
then replace the body of getInitiationResponse and the DetailedConsentResource
variant to call the helper so both reuse the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java`:
- Around line 452-459: The buildConsentResource method currently autounboxes
nullable fields from ExternalAPIBasicConsentResourceResponseDTO (e.g.,
getFrequency(), getValidityTime(), getRecurringIndicator(), getStatus(), and
potentially getReceipt()), which can cause NPEs for partially populated external
responses; before constructing ConsentResource validate that these boxed values
are non-null (or apply safe defaults) and throw a clear IllegalArgumentException
or return a controlled validation error if any required fields are missing, and
also handle a null receipt from getReceipt() (e.g., treat as empty JSON or
validated error) so ConsentResource(...) is only called with fully validated,
non-null primitive arguments.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.java`:
- Around line 55-57: The JSON parsing of consentResource.getReceipt() in
ExternalAPIBasicConsentResourceRequestDTO is unguarded and can throw
JSONException, aborting updates; wrap the JSONObject construction and toMap()
call in a try-catch that catches org.json.JSONException (or Exception), log or
warn about the malformed receipt (including consent id/context) and gracefully
handle it by leaving this.receipt null or an empty map so the flow continues;
apply the same defensive try-catch and handling to the analogous parsing in
ExternalAPIConsentResourceRequestDTO (the block around lines 63–68).

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`:
- Around line 523-529: The branch that checks
storedConsentResource.getClientID() vs consentManageData.getClientId() leaks
existence by logging NO_CONSENT_FOR_CLIENT_ERROR but throwing
CLIENT_ID_MISMATCH_ERROR; change the thrown ConsentException to use the generic
NO_CONSENT_FOR_CLIENT_ERROR (use
ConsentManageConstants.NO_CONSENT_FOR_CLIENT_ERROR) so both the log and the
thrown error are identical and non-revealing (adjust the ConsentException
constructor call that currently uses
ConsentManageConstants.CLIENT_ID_MISMATCH_ERROR, keeping
ResponseStatus.BAD_REQUEST and ConsentOperationEnum.CONSENT_UPDATE).
- Around line 517-520: The PUT/update error paths in DefaultConsentManageHandler
are mislabeling update failures as delete/create operations; locate where
storedConsentResource is checked and ConsentException is thrown (uses
ConsentException(ResponseStatus.BAD_REQUEST, "Consent not found",
ConsentOperationEnum.CONSENT_DELETE)) and change the operation enum to
ConsentOperationEnum.CONSENT_UPDATE, and similarly update other
throw/catch/helper usages in the same class that currently use CONSENT_CREATE or
CONSENT_DELETE for PUT handling (including the blocks around the
storedConsentResource checks and the exception wrapping logic) so they
consistently use CONSENT_UPDATE and update the error messages from “creating the
consent” to “updating the consent.”
- Around line 552-559: The new ConsentResource instance "updatingConsent" is
missing the stored consent ID so ConsentCoreServiceImpl.updateConsent rejects
it; before calling consentCoreService.updateConsent(updatingConsent) set the
consent ID from storedConsentResource (e.g., call
updatingConsent.setConsentId(storedConsentResource.getConsentId()) or
equivalent) so the update includes the original consent identifier; ensure this
is done in the non-pre-extension branch in DefaultConsentManageHandler where
updatingConsent is constructed from consentManageData and storedConsentResource.
- Around line 470-499: The PUT handling path in DefaultConsentManageHandler is
missing the header validation step (validateRequestHeaders), allowing invalid
requests to proceed; add a call to the same validateRequestHeaders(...) used by
POST/DELETE before any processing (i.e., before checking clientId and
requestPath and before extracting consentId), and ensure it uses the same
operation context (ConsentOperationEnum.CONSENT_UPDATE) and consentManageData
object so invalid headers are rejected early.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`:
- Around line 308-313: The isInternalConsentRequest method currently checks
headers.containsKey with ConsentManageConstants.INTERNAL_API_REQUEST_HEADER
which is case-sensitive; change it to perform a case-insensitive lookup by
iterating the headers map (from consentManageData.getHeaders()) and matching
keys with ConsentManageConstants.INTERNAL_API_REQUEST_HEADER using
equalsIgnoreCase, then parse and return Boolean.parseBoolean of the matched
value (handle nulls by returning false); update only isInternalConsentRequest to
use this key comparison so any header case (e.g., "X-WSO2-Internal-Request") is
recognized.
- Around line 476-480: In ConsentManageUtils update the assignment for the
authorization type so it uses the authorization type field instead of the
status: replace the call to updatedAuthResource.getAuthorizationStatus() passed
to authorization.setType(...) with the appropriate getter for the authorisation
type (e.g., updatedAuthResource.getAuthorizationType()), ensuring the variable
names authorization and updatedAuthResource remain unchanged.
- Around line 427-430: The code currently collects all mapping IDs from
storedResource.getConsentMappingResources() and deactivates them, which disables
mappings for sibling authorisations; update the logic to only collect mapping
IDs whose mapping resource is tied to the current authorization (compare the
mapping resource's authorization id to the authId in scope) before calling
consentCoreService.deactivateAccountMappings; specifically, filter
storedResource.getConsentMappingResources() by mappingResource.getAuthId() (or
equivalent getter) == authId and only add mappingResource.getMappingID() for
those matches.
- Around line 390-391: The catch block in ConsentManageUtils currently catches
ConsentManagementException and always rethrows a ConsentException with
ResponseStatus.BAD_REQUEST (see the catch handling that throws new
ConsentException(ResponseStatus.BAD_REQUEST, e.getMessage(),
ConsentOperationEnum.CONSENT_UPDATE)); change this so only request-validation
errors are mapped to BAD_REQUEST and service/DAO/backend failures are preserved
or translated to server errors: detect whether the caught
ConsentManagementException represents a client validation failure (by checking
its error code/message or a specific type/flag) and if so throw ConsentException
with ResponseStatus.BAD_REQUEST, otherwise either rethrow the original
ConsentManagementException or throw a ConsentException with
ResponseStatus.INTERNAL_SERVER_ERROR (or appropriate server status) so backend
failures are not collapsed into 400s.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java`:
- Around line 197-204: The
callExternalService(ExternalAPIPostConsentUpdateRequestDTO) method incorrectly
constructs the request payload via new JSONObject(requestDTO), bypassing
ExternalAPIPostConsentUpdateRequestDTO's custom serialization; update the method
to use requestDTO.toJson() when building requestJson so the nested
consentResource is constructed as intended (keep using
ServiceExtensionTypeEnum.ENRICH_CONSENT_UPDATE_RESPONSE and gson.fromJson on the
response as before).

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java`:
- Around line 166-270: These negative tests currently only assert inside the
catch for ConsentException, so if defaultConsentManageHandler.handlePost(...)
stops throwing the test will pass silently; update each test (e.g.,
testHandlePostWithoutClientId, testHandlePostWithInvalidHeaders,
testHandlePostWithoutRequestPath, testHandlePostWithInvalidRequestPath,
testHandlePostWithInvalidPayload, testHandlePostWithInvalidInitiation) to
explicitly fail when no ConsentException is thrown by calling Assert.fail (or
equivalent) immediately after invoking
defaultConsentManageHandler.handlePost(consentManageDataMock), keeping the
existing catch block assertions for e.getPayload() intact so the test fails when
no exception occurs and still validates the error when one is thrown.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java`:
- Around line 296-313: Replace the prior use of getConsent() (which opens a
separate transaction) with loading the existing row via the DAO on the current
connection before the update (e.g., call
consentCoreDAO.getConsentResourceById(connection,
consentResource.getConsentID()) or the DAO method that returns the persisted
ConsentResource on that connection), fail cleanly if it returns null, then call
consentCoreDAO.updateConsentResource(connection, consentResource), and for the
audit use the persisted snapshot values (the persisted clientId and previous
status) in the consentDataMap and postStateChange call instead of
consentResource.getClientID() and previousConsent.getCurrentStatus(); finally
reload the updated consent from the same DAO/connection after commit (or before
returning) and return that reloaded updated consent.

---

Nitpick comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java`:
- Around line 123-143: The getInitiationResponse(Object responseObj,
ConsentResource createdConsent) implementation duplicates the same JSON mutation
performed by the DetailedConsentResource overload; extract the shared logic into
a private helper (e.g., populateInitiationData or buildInitiationData) that
accepts the JSONObject response (or its dataObject) and the ConsentResource,
moves the put(...) calls for CONSENT_ID, CREATION_DATE_TIME,
STATUS_UPDATE_DATE_TIME and STATUS, and the remove/put of DATA into that helper,
then replace the body of getInitiationResponse and the DetailedConsentResource
variant to call the helper so both reuse the single implementation.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/DataProviders.java`:
- Around line 48-106: The four unused payload constants in DataProviders
(INITIATION_WITHOUT_PERMISSIONS, INITIATION_WITH_NON_STRING_PERMISSION,
INITIATION_WITHOUT_TRANS_FROM_DATES, INITIATION_WITHOUT_TRANS_TO_DATES) should
be either removed or exercised by tests; update the DataProviders class
accordingly by deleting the unused constants if they’re obsolete, or add unit
tests that reference these constants (e.g., in the consent validation test
suite) to cover missing-permissions, non-string-permission, missing
TransactionFromDateTime, and missing TransactionToDateTime scenarios so the
constants are actually used.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 33dc55f0-6be9-404b-98d2-5f114ee2cc50

📥 Commits

Reviewing files that changed from the base of the PR and between 52288bc and d5e2570.

📒 Files selected for processing (20)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/extension/model/ServiceExtensionTypeEnum.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceResponseDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ExternalAPIPostConsentUpdateRequestDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ExternalAPIPreConsentUpdateRequestDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ExternalAPIPreConsentUpdateResponseDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageConstants.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/DataProviders.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestUtil.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/ConsentCoreService.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/constants/ConsentCoreServiceConstants.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

♻️ Duplicate comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java (1)

592-595: ⚠️ Potential issue | 🟡 Minor

Inconsistent error message in log statement.

The log says "creating the consent" but this is the PUT handler for updating. The exception message correctly says "updating".

Suggested fix
         } catch (ConsentManagementException e) {
-            log.error("Error Occurred while creating the consent", e);
+            log.error("Error Occurred while updating the consent", e);
             throw new ConsentException(ResponseStatus.INTERNAL_SERVER_ERROR,
                     "Error Occurred while updating the consent", ConsentOperationEnum.CONSENT_UPDATE);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`
around lines 592 - 595, The catch block for ConsentManagementException in
DefaultConsentManageHandler currently logs "Error Occurred while creating the
consent" which is inconsistent with the update flow; change the log.error
message in that catch (ConsentManagementException e) block to "Error Occurred
while updating the consent" (matching the ConsentException message) so the log
and thrown ConsentException both refer to the update operation.
🧹 Nitpick comments (3)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ConsentManageAuthorizationUpdateDTO.java (1)

28-34: Inconsistent access modifier on mappingResources field.

All other fields use private access, but mappingResources has package-private (default) access. This should be private for consistency and proper encapsulation.

Suggested fix
     private String authorizationType;
     private long updatedTime;
-    List<MappingResource> mappingResources;
+    private List<MappingResource> mappingResources;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ConsentManageAuthorizationUpdateDTO.java`
around lines 28 - 34, The field mappingResources in
ConsentManageAuthorizationUpdateDTO currently has package-private access while
all other fields are private; change its declaration to private
List<MappingResource> mappingResources to match encapsulation, and ensure any
existing getter/setter methods (e.g., getMappingResources/setMappingResources)
reference the renamed private field if needed.
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java (1)

123-149: Code duplication with existing getInitiationResponse overload.

This new method duplicates lines 108-121 except for the parameter type. Consider extracting the common logic into a private helper that accepts the individual fields (consentID, createdTime, updatedTime, currentStatus) to reduce duplication.

Example refactor
private static JSONObject buildInitiationResponse(Object responseObj, String consentId, 
        long createdTime, long updatedTime, String status) {
    JSONObject response = (JSONObject) responseObj;
    JSONObject dataObject = response.getJSONObject(ConsentExtensionConstants.DATA);
    dataObject.put(ConsentExtensionConstants.CONSENT_ID, consentId);
    dataObject.put(ConsentExtensionConstants.CREATION_DATE_TIME, convertToISO8601(createdTime));
    dataObject.put(ConsentExtensionConstants.STATUS_UPDATE_DATE_TIME, convertToISO8601(updatedTime));
    dataObject.put(ConsentExtensionConstants.STATUS, status);
    response.remove(ConsentExtensionConstants.DATA);
    response.put(ConsentExtensionConstants.DATA, dataObject);
    return response;
}

public static JSONObject getInitiationResponse(Object responseObj, DetailedConsentResource createdConsent) {
    return buildInitiationResponse(responseObj, createdConsent.getConsentID(),
            createdConsent.getCreatedTime(), createdConsent.getUpdatedTime(), createdConsent.getCurrentStatus());
}

public static JSONObject getInitiationResponse(Object responseObj, ConsentResource createdConsent) {
    JSONObject result = buildInitiationResponse(responseObj, createdConsent.getConsentID(),
            createdConsent.getCreatedTime(), createdConsent.getUpdatedTime(), createdConsent.getCurrentStatus());
    if (log.isDebugEnabled()) {
        log.debug(String.format("Initiation response constructed for consent ID: %s with status: %s",
                createdConsent.getConsentID().replaceAll("[\r\n]", ""),
                createdConsent.getCurrentStatus().replaceAll("[\r\n]", "")));
    }
    return result;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java`
around lines 123 - 149, The two getInitiationResponse methods duplicate logic;
extract the common JSON construction into a private helper (e.g.,
buildInitiationResponse) that takes (Object responseObj, String consentId, long
createdTime, long updatedTime, String status) and performs the JSONObject
dataObject population and replacement, then have both public methods call this
helper (getInitiationResponse(Object, DetailedConsentResource) and
getInitiationResponse(Object, ConsentResource) pass their respective
createdConsent fields into buildInitiationResponse); keep the existing debug log
only in the ConsentResource overload after calling the helper.
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java (1)

508-516: Path check using contains() is fragile.

The check consentManageData.getRequestPath().contains("authorisations") matches any path containing that substring. Consider using a regex or structured path parsing for more precise routing (e.g., matching the pattern */authorisations/*).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`
around lines 508 - 516, The request path check using
consentManageData.getRequestPath().contains("authorisations") is too permissive;
update the conditional in DefaultConsentManageHandler so it matches the intended
route pattern (e.g., segment-based or regex like .*/authorisations/.*) instead
of a substring match; locate the block that calls
ConsentManageUtils.isInternalConsentRequest(...) and replace the contains(...)
check with a precise match (regex or parsed path segment comparison) so only
requests matching the pattern /authorisations/ (as a path segment) trigger
ConsentManageUtils.updateConsentAuthResource and the subsequent response
handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`:
- Line 11: The embedded OpenAPI document still lists version: v1.0.3 while the
file name indicates v1.0.4—update the OpenAPI info.version field by changing the
version key (the top-level "version:"/info.version entry in
accelerator-extensions-v1.0.4.yaml) to v1.0.4 so docs and generated clients
reflect the correct contract version.
- Around line 2673-2692: The schema FailedResponseApplicationProcess currently
lists a required "data" property that is not defined and defines errorMessage
via a separate object schema (FailedResponseApplicationProcessData) while
examples show a top-level string; remove the nonexistent "data" requirement from
FailedResponseApplicationProcess, change its errorMessage property to type:
string (or inline a string description) instead of $ref to
FailedResponseApplicationProcessData, and delete or simplify the
FailedResponseApplicationProcessData schema (or update references) so the
contract validates failure payloads where "status" is "ERROR" and "errorMessage"
is a plain string.
- Around line 1417-1429: The schema RequestForPreProcessConsentUpdate currently
defines the property storedConsentResource but the DTO
ExternalAPIPreConsentUpdateRequestDTO uses consentResource; rename the YAML
property storedConsentResource to consentResource (and update any $ref usages if
present) so the OpenAPI schema field matches the DTO name and enables correct
deserialization into ExternalAPIPreConsentUpdateRequestDTO.consentResource.
- Around line 2109-2111: Stored consent schemas StoredBasicConsentResourceData
and StoredDetailedConsentResourceData currently define the validityTime property
as type: integer with format: int32; update both definitions so validityTime
uses format: int64 to match ExternalAPIBasicConsentResourceResponseDTO (Long)
and the non-stored consent schemas, preventing downcasting in generated
clients—locate the validityTime field in StoredBasicConsentResourceData and
StoredDetailedConsentResourceData and change format from int32 to int64.
- Around line 2464-2469: The OpenAPI schema for basicConsentData is missing the
array type for additionalProperties: update the basicConsentData object so its
additionalProperties explicitly declares type: array and retains the existing
items: { type: string } so each map value is defined as an array of strings;
locate the basicConsentData definition and add type: array under
additionalProperties (keeping items: type: string) to ensure tools generate
Map<String, List<String>> correctly.
- Around line 836-840: The request body for the operation referencing
PopulateConsentAuthorizeScreenRequestBody is currently treated as optional; set
requestBody.required: true for that operation so generated clients must supply
the payload. Locate the operation that uses the schema reference
'#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' and add a
top-level requestBody.required: true alongside the existing content block
(ensuring the schema name PopulateConsentAuthorizeScreenRequestBody remains
unchanged).

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/queries/ConsentMgtCommonDBQueries.java`:
- Around line 44-45: The LEFT JOIN change can produce rows with
ATT_KEY/ATT_VALUE = NULL and the mapper
ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes currently
unconditionally does put(getString(ATT_KEY), getString(ATT_VALUE)), producing
{null=null}; update the mapper to guard against null/blank keys (and optionally
null values) by skipping any ResultSet row where ATT_KEY is null/blank before
calling put, so ConsentCoreDAOImpl.getConsentResourceWithAttributes can safely
use the LEFT JOIN SQL in ConsentMgtCommonDBQueries without creating malformed
attribute entries.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`:
- Around line 361-378: The sequence of non-atomic calls
(consentCoreService.updateAuthorizationResources,
consentCoreService.updateAuthorizationUser, deactivateAccountMappings,
createConsentMappings, then constructUpdateResponse) can leave data inconsistent
if an intermediate call fails; refactor by moving this whole sequence into a
single transactional service method on ConsentCoreService (e.g., add
updateAuthorizationWithMappings(authId, updatingAuthResource, mappingResources)
or similar) that performs updateAuthorizationResources and
updateAuthorizationUser, deactivates and creates mappings inside one DB
transaction and either commits or rolls back, or implement compensating rollback
logic inside that service method to undo prior changes on failure; update the
call site to invoke the new transactional method and return its result (use
AuthorizationResource and List<ConsentMappingResource> handling inside the new
service method).
- Around line 407-416: The extractAuthIdFromPath method can return extra path
segments (e.g., "{authId}/something"); update extractAuthIdFromPath to, after
splitting requestPath by "/authorisations/" and assigning pathParts[1], further
split that segment by "/" and take the first element as the authId (trim it),
then validate it as before; ensure you still log the same error message via
log.error and throw ConsentManagementException when authId is blank or missing.
Use the existing method name extractAuthIdFromPath and the local variable authId
to locate and apply this change.

---

Duplicate comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`:
- Around line 592-595: The catch block for ConsentManagementException in
DefaultConsentManageHandler currently logs "Error Occurred while creating the
consent" which is inconsistent with the update flow; change the log.error
message in that catch (ConsentManagementException e) block to "Error Occurred
while updating the consent" (matching the ConsentException message) so the log
and thrown ConsentException both refer to the update operation.

---

Nitpick comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java`:
- Around line 123-149: The two getInitiationResponse methods duplicate logic;
extract the common JSON construction into a private helper (e.g.,
buildInitiationResponse) that takes (Object responseObj, String consentId, long
createdTime, long updatedTime, String status) and performs the JSONObject
dataObject population and replacement, then have both public methods call this
helper (getInitiationResponse(Object, DetailedConsentResource) and
getInitiationResponse(Object, ConsentResource) pass their respective
createdConsent fields into buildInitiationResponse); keep the existing debug log
only in the ConsentResource overload after calling the helper.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`:
- Around line 508-516: The request path check using
consentManageData.getRequestPath().contains("authorisations") is too permissive;
update the conditional in DefaultConsentManageHandler so it matches the intended
route pattern (e.g., segment-based or regex like .*/authorisations/.*) instead
of a substring match; locate the block that calls
ConsentManageUtils.isInternalConsentRequest(...) and replace the contains(...)
check with a precise match (regex or parsed path segment comparison) so only
requests matching the pattern /authorisations/ (as a path segment) trigger
ConsentManageUtils.updateConsentAuthResource and the subsequent response
handling.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ConsentManageAuthorizationUpdateDTO.java`:
- Around line 28-34: The field mappingResources in
ConsentManageAuthorizationUpdateDTO currently has package-private access while
all other fields are private; change its declaration to private
List<MappingResource> mappingResources to match encapsulation, and ensure any
existing getter/setter methods (e.g., getMappingResources/setMappingResources)
reference the renamed private field if needed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9a8a52dd-a29d-45df-8aff-06425356e8b6

📥 Commits

Reviewing files that changed from the base of the PR and between d5e2570 and 8fd5018.

📒 Files selected for processing (10)
  • financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/queries/ConsentMgtCommonDBQueries.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceResponseDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ConsentManageAuthorizationUpdateDTO.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
🚧 Files skipped from review as they are similar to previous changes (3)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java

license:
name: Apache 2.0
url: https://www.apache.org/licenses/LICENSE-2.0.html
version: v1.0.3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Bump info.version to v1.0.4.

The file is published as accelerator-extensions-v1.0.4.yaml, but the embedded OpenAPI version still says v1.0.3. That will surface the wrong contract version in docs and generated clients.

Suggested fix
-  version: v1.0.3
+  version: v1.0.4
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
version: v1.0.3
version: v1.0.4
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
at line 11, The embedded OpenAPI document still lists version: v1.0.3 while the
file name indicates v1.0.4—update the OpenAPI info.version field by changing the
version key (the top-level "version:"/info.version entry in
accelerator-extensions-v1.0.4.yaml) to v1.0.4 so docs and generated clients
reflect the correct contract version.

Comment on lines +836 to +840
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In OpenAPI 3.0.x, if an operation omits requestBody.required: true, is the request body treated as optional by default for validators and generated clients?

💡 Result:

Yes. In OpenAPI 3.0.x, requestBody.required defaults to false, so if you omit requestBody.required: true, the request body is optional by default and spec-compliant validators and generated clients should treat it as optional. [1][2]

Sources: OpenAPI 3.0.1 spec (Request Body Object: “Defaults to false”) [1]; Swagger/OpenAPI 3.0 requestBody docs (“optional … false by default”) [2].


Make this request body mandatory.

In OpenAPI 3.0.x, requestBody.required defaults to false when omitted. The PopulateConsentAuthorizeScreenRequestBody schema requires requestId and data, but this operation publishes the request body as optional, allowing generated clients to call it without a payload.

Suggested fix
   /populate-consent-authorize-screen:
     post:
       tags:
         - Consent
       summary: handle validations before consent  authorization and consent data to load in consent authorization UI
       requestBody:
         content:
           application/json:
             schema:
               $ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody'
+        required: true
       responses:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody'
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody'
required: true
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 836 - 840, The request body for the operation referencing
PopulateConsentAuthorizeScreenRequestBody is currently treated as optional; set
requestBody.required: true for that operation so generated clients must supply
the payload. Locate the operation that uses the schema reference
'#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' and add a
top-level requestBody.required: true alongside the existing content block
(ensuring the schema name PopulateConsentAuthorizeScreenRequestBody remains
unchanged).

Comment on lines +1417 to +1429
RequestForPreProcessConsentUpdate:
type: object
properties:
consentUpdateData:
type: object
description: The upload payload used by third parties which includes detailed information on data access request.
storedConsentResource:
$ref: '#/components/schemas/StoredBasicConsentResourceData'
requestHeaders:
$ref: '#/components/schemas/RequestHeaders'
consentResourcePath:
type: string
description: To identify requested consent type
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')
dto=$(fd 'ExternalAPIPreConsentUpdateRequestDTO\.java$')

sed -n '1417,1430p' "$spec"
echo '---'
sed -n '28,76p' "$dto"

Repository: wso2/financial-services-accelerator

Length of output: 2349


Rename storedConsentResource to consentResource to match the DTO field name.

The schema defines storedConsentResource, but ExternalAPIPreConsentUpdateRequestDTO.java exposes this field as consentResource. When the framework deserializes incoming JSON to the DTO, the mismatch will cause the payload to be dropped or left null, breaking the consent update flow.

Suggested fix
     RequestForPreProcessConsentUpdate:
       type: object
       properties:
         consentUpdateData:
           type: object
           description: The upload payload used by third parties which includes detailed information on data access request.
-        storedConsentResource:
+        consentResource:
           $ref: '#/components/schemas/StoredBasicConsentResourceData'
         requestHeaders:
           $ref: '#/components/schemas/RequestHeaders'
         consentResourcePath:
           type: string
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
RequestForPreProcessConsentUpdate:
type: object
properties:
consentUpdateData:
type: object
description: The upload payload used by third parties which includes detailed information on data access request.
storedConsentResource:
$ref: '#/components/schemas/StoredBasicConsentResourceData'
requestHeaders:
$ref: '#/components/schemas/RequestHeaders'
consentResourcePath:
type: string
description: To identify requested consent type
RequestForPreProcessConsentUpdate:
type: object
properties:
consentUpdateData:
type: object
description: The upload payload used by third parties which includes detailed information on data access request.
consentResource:
$ref: '#/components/schemas/StoredBasicConsentResourceData'
requestHeaders:
$ref: '#/components/schemas/RequestHeaders'
consentResourcePath:
type: string
description: To identify requested consent type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 1417 - 1429, The schema RequestForPreProcessConsentUpdate currently
defines the property storedConsentResource but the DTO
ExternalAPIPreConsentUpdateRequestDTO uses consentResource; rename the YAML
property storedConsentResource to consentResource (and update any $ref usages if
present) so the OpenAPI schema field matches the DTO name and enables correct
deserialization into ExternalAPIPreConsentUpdateRequestDTO.consentResource.

Comment on lines +2109 to +2111
validityTime:
type: integer
format: int32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')
dto=$(fd 'ExternalAPIBasicConsentResourceResponseDTO\.java$')

sed -n '2088,2140p' "$spec"
echo '---'
sed -n '26,90p' "$dto"

Repository: wso2/financial-services-accelerator

Length of output: 2728


🏁 Script executed:

#!/bin/bash
set -euo pipefail

spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')

# Show lines 2109-2111 to verify the first location
echo "=== Lines 2109-2111 (first location) ==="
sed -n '2109,2111p' "$spec"

# Show lines 2137-2139 to verify the second location
echo "=== Lines 2137-2139 (second location) ==="
sed -n '2137,2139p' "$spec"

# Search for non-stored consent schemas (BasicConsentResourceData and DetailedConsentResourceData)
echo "=== Searching for BasicConsentResourceData ==="
grep -n "BasicConsentResourceData:" "$spec" | head -5

echo "=== Searching for DetailedConsentResourceData ==="
grep -n "DetailedConsentResourceData:" "$spec" | head -5

Repository: wso2/financial-services-accelerator

Length of output: 556


🏁 Script executed:

#!/bin/bash
set -euo pipefail

spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')

# Show BasicConsentResourceData including validityTime field
echo "=== BasicConsentResourceData (lines 2022-2087) ==="
sed -n '2022,2087p' "$spec" | grep -A 2 'validityTime'

# Show DetailedConsentResourceData including validityTime field
echo "=== DetailedConsentResourceData (lines 2040-2115) ==="
sed -n '2040,2115p' "$spec" | grep -A 2 'validityTime'

Repository: wso2/financial-services-accelerator

Length of output: 615


Use int64 for stored consent validityTime.

ExternalAPIBasicConsentResourceResponseDTO.validityTime is Long, and the non-stored consent schemas here already use format: int64. Keeping the stored variants at int32 will downcast the field in generated clients for the update flow.

Suggested fix
         validityTime:
           type: integer
-          format: int32
+          format: int64

Apply that change to both StoredBasicConsentResourceData (lines 2109-2111) and StoredDetailedConsentResourceData (lines 2137-2139).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
validityTime:
type: integer
format: int32
validityTime:
type: integer
format: int64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 2109 - 2111, Stored consent schemas StoredBasicConsentResourceData
and StoredDetailedConsentResourceData currently define the validityTime property
as type: integer with format: int32; update both definitions so validityTime
uses format: int64 to match ExternalAPIBasicConsentResourceResponseDTO (Long)
and the non-stored consent schemas, preventing downcasting in generated
clients—locate the validityTime field in StoredBasicConsentResourceData and
StoredDetailedConsentResourceData and change format from int32 to int64.

Comment on lines +2464 to +2469
basicConsentData:
type: object
description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section.
additionalProperties:
items:
type: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In OpenAPI 3.0.x, how do you define an object whose arbitrary property values are arrays of strings? Is additionalPropertieswith onlyitemsvalid withouttype: array?

💡 Result:

To model “an object with arbitrary keys whose values are arrays of strings” in OpenAPI 3.0.x, define the object as type: object and put an array schema under additionalProperties:

schema:
  type: object
  additionalProperties:
    type: array
    items:
      type: string

additionalProperties can be a Schema Object, so using an array schema there is the intended approach. [1]

If you write only:

additionalProperties:
  items:
    type: string

…it’s not the right way to say “the values are arrays”. In OAS 3.0.x, items is the array-item schema and is tied to type: array (the spec calls out that items MUST be present if type is array, and tooling commonly flags items without type: array as a type mismatch). [1][2][3]


Add type: array to additionalProperties in basicConsentData.

The schema describes each section key mapping to a list of bullet points, but the OpenAPI definition is incomplete. In OpenAPI 3.0.x, additionalProperties must include type: array to declare that property values are arrays; items alone without type: array is not a valid way to specify this. Code generators will not correctly model this as Map<String, List<String>> in its current form.

Suggested fix
             basicConsentData:
               type: object
               description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section.
               additionalProperties:
+                type: array
                 items:
                   type: string
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
basicConsentData:
type: object
description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section.
additionalProperties:
items:
type: string
basicConsentData:
type: object
description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section.
additionalProperties:
type: array
items:
type: string
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 2464 - 2469, The OpenAPI schema for basicConsentData is missing the
array type for additionalProperties: update the basicConsentData object so its
additionalProperties explicitly declares type: array and retains the existing
items: { type: string } so each map value is defined as an array of strings;
locate the basicConsentData definition and add type: array under
additionalProperties (keeping items: type: string) to ensure tools generate
Map<String, List<String>> correctly.

Comment on lines +2673 to +2692
required:
- data
- status
type: object
properties:
status:
type: string
description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR."
enum:
- ERROR
errorMessage:
$ref: '#/components/schemas/FailedResponseApplicationProcessData'
FailedResponseApplicationProcessData:
required:
- errorMessage
type: object
properties:
errorMessage:
type: string
description: Error message to be returned
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Align FailedResponseApplicationProcess with its own examples.

This schema requires a data property that it never defines, and it types errorMessage as an object even though the application examples return a top-level string. As written, the contract cannot validate the documented failure payloads.

Suggested fix
     FailedResponseApplicationProcess:
       required:
-        - data
+        - errorMessage
         - status
       type: object
       properties:
         status:
           type: string
           description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR."
           enum:
             - ERROR
         errorMessage:
-          $ref: '#/components/schemas/FailedResponseApplicationProcessData'
+          type: string
+          description: Error message to be returned
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
required:
- data
- status
type: object
properties:
status:
type: string
description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR."
enum:
- ERROR
errorMessage:
$ref: '#/components/schemas/FailedResponseApplicationProcessData'
FailedResponseApplicationProcessData:
required:
- errorMessage
type: object
properties:
errorMessage:
type: string
description: Error message to be returned
FailedResponseApplicationProcess:
required:
- errorMessage
- status
type: object
properties:
status:
type: string
description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR."
enum:
- ERROR
errorMessage:
type: string
description: Error message to be returned
FailedResponseApplicationProcessData:
required:
- errorMessage
type: object
properties:
errorMessage:
type: string
description: Error message to be returned
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 2673 - 2692, The schema FailedResponseApplicationProcess currently
lists a required "data" property that is not defined and defines errorMessage
via a separate object schema (FailedResponseApplicationProcessData) while
examples show a top-level string; remove the nonexistent "data" requirement from
FailedResponseApplicationProcess, change its errorMessage property to type:
string (or inline a string description) instead of $ref to
FailedResponseApplicationProcessData, and delete or simplify the
FailedResponseApplicationProcessData schema (or update references) so the
contract validates failure payloads where "status" is "ERROR" and "errorMessage"
is a plain string.

Comment on lines +44 to 45
"FS_CONSENT_ATTRIBUTE.ATT_KEY, FS_CONSENT_ATTRIBUTE.ATT_VALUE FROM FS_CONSENT LEFT JOIN " +
"FS_CONSENT_ATTRIBUTE ON FS_CONSENT.CONSENT_ID = FS_CONSENT_ATTRIBUTE.CONSENT_ID WHERE FS_CONSENT" +
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle null attribute rows before switching this query to LEFT JOIN.

With this join, a consent that has no rows in FS_CONSENT_ATTRIBUTE now comes back as a single row with ATT_KEY/ATT_VALUE = NULL. ConsentCoreDAOImpl.getConsentResourceWithAttributes(...) passes that ResultSet straight into ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes(...), and that mapper currently does an unconditional put(getString(ATT_KEY), getString(ATT_VALUE)). The result is a malformed attributes map ({null=null}) instead of an empty one for attribute-less consents. Please either guard null/blank keys in the mapper or keep the old join semantics until that mapper is fixed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/queries/ConsentMgtCommonDBQueries.java`
around lines 44 - 45, The LEFT JOIN change can produce rows with
ATT_KEY/ATT_VALUE = NULL and the mapper
ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes currently
unconditionally does put(getString(ATT_KEY), getString(ATT_VALUE)), producing
{null=null}; update the mapper to guard against null/blank keys (and optionally
null values) by skipping any ResultSet row where ATT_KEY is null/blank before
calling put, so ConsentCoreDAOImpl.getConsentResourceWithAttributes can safely
use the LEFT JOIN SQL in ConsentMgtCommonDBQueries without creating malformed
attribute entries.

Comment on lines +361 to +378
if (matchingAuthResource.isPresent()) {
// Update the authorization resource if authId is found
consentCoreService.updateAuthorizationResources(Collections.singletonList(updatingAuthResource));
consentCoreService.updateAuthorizationUser(authId, updatingAuthResource.getUserID());
if (log.isDebugEnabled()) {
log.debug(String.format("Authorization resource updated successfully for auth id: %s",
authId.replaceAll("[\r\n]", "")));
}
AuthorizationResource updatedAuthResource = consentCoreService.getAuthorizationResource(authId);
List<ConsentMappingResource> createdMappingResources = null;
if (CollectionUtils.isNotEmpty(authDetails.getMappingResources())) {
// Deactivate the current account mappings of the authorization resource if available
deactivateAccountMappings(detailedConsentResource, consentCoreService, authId);
// Create new account mappings based on the request payload
createdMappingResources = createConsentMappings(authDetails.getMappingResources(),
consentCoreService, authId);
}
return constructUpdateResponse(updatedAuthResource, createdMappingResources);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Non-atomic update sequence could leave data in an inconsistent state.

The authorization update involves multiple sequential service calls:

  1. updateAuthorizationResources (line 363)
  2. updateAuthorizationUser (line 364)
  3. deactivateAccountMappings (line 373)
  4. createConsentMappings (lines 375-376)

If any intermediate call fails (e.g., deactivateAccountMappings fails after auth resource is updated), the consent data could be left partially updated with no rollback.

Consider wrapping these operations in a single transactional method in the service layer, or implementing compensating actions on failure.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`
around lines 361 - 378, The sequence of non-atomic calls
(consentCoreService.updateAuthorizationResources,
consentCoreService.updateAuthorizationUser, deactivateAccountMappings,
createConsentMappings, then constructUpdateResponse) can leave data inconsistent
if an intermediate call fails; refactor by moving this whole sequence into a
single transactional service method on ConsentCoreService (e.g., add
updateAuthorizationWithMappings(authId, updatingAuthResource, mappingResources)
or similar) that performs updateAuthorizationResources and
updateAuthorizationUser, deactivates and creates mappings inside one DB
transaction and either commits or rolls back, or implement compensating rollback
logic inside that service method to undo prior changes on failure; update the
call site to invoke the new transactional method and return its result (use
AuthorizationResource and List<ConsentMappingResource> handling inside the new
service method).

Comment on lines +407 to +416
private static String extractAuthIdFromPath(String requestPath) throws ConsentManagementException {

String[] pathParts = requestPath.split("/authorisations/");
String authId = pathParts.length > 1 ? pathParts[1] : null;
if (StringUtils.isBlank(authId)) {
log.error("Auth id is missing in the request path.");
throw new ConsentManagementException("Invalid request path: Auth id is missing.");
}
return authId;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

extractAuthIdFromPath may include trailing path segments.

If the request path has segments after the auth ID (e.g., /consent/{id}/authorisations/{authId}/something), splitting by /authorisations/ and taking pathParts[1] will return {authId}/something instead of just {authId}.

Suggested fix to extract only the auth ID
 private static String extractAuthIdFromPath(String requestPath) throws ConsentManagementException {

     String[] pathParts = requestPath.split("/authorisations/");
-    String authId = pathParts.length > 1 ? pathParts[1] : null;
+    String authId = pathParts.length > 1 ? pathParts[1].split("/")[0] : null;
     if (StringUtils.isBlank(authId)) {
         log.error("Auth id is missing in the request path.");
         throw new ConsentManagementException("Invalid request path: Auth id is missing.");
     }
     return authId;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`
around lines 407 - 416, The extractAuthIdFromPath method can return extra path
segments (e.g., "{authId}/something"); update extractAuthIdFromPath to, after
splitting requestPath by "/authorisations/" and assigning pathParts[1], further
split that segment by "/" and take the first element as the authId (trim it),
then validate it as before; ensure you still log the same error message via
log.error and throw ConsentManagementException when authId is blank or missing.
Use the existing method name extractAuthIdFromPath and the local variable authId
to locate and apply this change.

@Ashi1993 Ashi1993 closed this Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants